Claude finished @fabrizzio-dotCMS's task in 2m 58s —— View job

PR Review

• Read changed files
• Analyze for correctness, edge cases, design issues
• Post review findings

Findings

1. INFO level silently drops the Throwable — IndexConfigHelper.java:62

case "INFO":  Logger.info(clazz, message);    break;   // t is silently dropped

Logger.info has no Throwable overload (confirmed: only info(Class, String) exists). All other cases (DEBUG, WARN, ERROR) pass t correctly. The Javadoc says @param t the throwable, or null if none, implying callers expect it to be used. An operator who sets DOTCMS_SHADOW_WRITE_LOG_LEVEL=INFO to reduce noise will silently lose stack traces for non-null throwables — with no warning. Four of the six call sites pass a non-null t.

Either document this in the Javadoc explicitly, or include the exception message inline:

case "INFO": Logger.info(clazz, t != null ? message + " — " + t.getMessage() : message); break;

Fix this →

2. writeBoolean sentinel primaryResult = true — PhaseRouter.java:238

boolean primaryResult = true;  // sentinel: optimistic default

The default value true is never observed in practice (primary is always in the providers list), but it's a misleading sentinel. If writeProviders() ever returns a list that doesn't include readProvider() (e.g., a future phase), the method silently returns true instead of throwing. false would be a safer pessimistic default, or use a Boolean (nullable) and throw if never set.

3. No test coverage for the log-level dispatch itself

PhaseRouterTest has 12 tests covering routing semantics, but none verify that logShadowWriteFailure actually routes to the correct level, or that the INFO case does/doesn't include the Throwable. This is the core behavioral change of the PR and it has no direct tests.

4. Config re-read on every logShadowWriteFailure call

final String level = Config.getStringProperty(SHADOW_WRITE_LOG_LEVEL_KEY, "WARN").toUpperCase();

In a bulk-processor failure storm (e.g., OS unavailable during a full reindex), this method could be called thousands of times per batch. Config.getStringProperty is not free. The hot-reload flexibility is nice, but consider whether it's worth the overhead on a hot failure path. If hot-reload isn't a requirement, cache the value. If it is, that should be documented.

5. queueApi = null in the full test constructor — ContentletIndexAPIImpl.java:215

this.queueApi = null; // not needed for the methods under test

If a future test exercises a path that calls queueApi.deleteReindexEntry() or queueApi.markAsFailed(), it gets a NPE with no useful message. Consider a fail-fast alternative (null object or explicit Objects.requireNonNull guard in the methods that use it, or a mock that throws a descriptive UnsupportedOperationException).

6. TOCTOU on phase reads (pre-existing, but amplified by this PR)

Several methods call MigrationPhase.current() multiple times within one operation — e.g., writeChecked calls writeProviders() and then readProvider() separately; isPhase2() calls isReadEnabled() and !isMigrationComplete(). Each call re-reads the config. A phase transition concurrent with a write operation could observe two different phases, routing to incorrect providers. This pre-dates this PR but the new tests would be a good place to document the assumption (phase is stable for the duration of a write).

Summary: The INFO/Throwable issue (point 1) is the clearest correctness gap introduced by this PR. Points 2, 5 are minor fragility concerns. Points 3 and 4 are worth a follow-up.